fix(automation): enforce hourly DDD development contract - #1545
fix(automation): enforce hourly DDD development contract#1545seonghobae wants to merge 26 commits into
Conversation
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@opencode-agent @cwl-noema-review Please review exact head |
|
@opencode-agent Please repair the current DDD-contract review findings on this existing owner branch and then synchronize current protected main non-destructively (no rebase/force).
Preserve the single existing hourly coordinator (no additional cron), its bounded dispatch budgets, exact default-branch/head revalidation, dedicated-writer lease behavior, and no |
|
@opencode-agent review The unsafe temporary source-fix workflow/helper have been removed at exact head |
|
The temporary write-capable source-fix workflow/helper were reintroduced by the subsequent |
|
@opencode-agent Please implement the current-head review remediation on exact head |
|
Current exact head |
|
@opencode-agent review exact head |
|
@opencode-agent current-main convergence pass. All visible Devin threads are resolved on |
|
The protected-main production failure in Organization Commercial Readiness Loop run |
|
Noema owner-path acceptance finding (2026-09-05 KST): protected central Please preserve #1545's central single-writer/DDD intent while ensuring the protected successor admission contract is provider-neutral: repository opt-in + DDD capability/prompt binding + manual dispatch/concurrency should be sufficient, and leaf products must not be required to expose NVIDIA/OpenAI/OpenRouter/Bytez provider credentials. Once that contract is protected, Noema can adopt the exact handshake and remove its local cron atomically with its existing workflow regression. Until then Noema retains its local scheduled writer rather than creating a writer outage or duplicating provider authority. |
|
Noema consumer prerequisite refresh (2026-09-05 KST): protected |
Three conflicts. One needed relocation rather than a choice: - .github/workflows/organization-commercial-readiness-loop-quality-ci.yml — modify/delete. Main deleted it in ec4521f ("consolidate commercial readiness quality"), which moved the contract into `agent-review-runtime-quality-ci.yml` (+33) and retired only the standalone thin caller (-33). This branch had edited the retired file to bring its two new modules into scope, so that intent was relocated into the surviving workflow: `organization_commercial_readiness_core.py` and `organization_commercial_readiness_ddd_contract.py` added to the `paths:` filter, to the changed-scope selector, and to the `compileall` list. The coverage `--include` was deliberately left at main's exact `'scripts/ci/organization_commercial_readiness_loop.py'`. Widening it to a glob broke four contract tests that pin that literal string (test_agent_review_runtime_quality_consolidation.py:202, test_exact_head_coverage_quality_gate_contract.py:118, test_organization_commercial_readiness_loop_policy.py:176, and the negative assertion at test_..._import_contract.py:24). Nothing is lost: the two new modules live under `scripts/ci`, which the repository-wide coverage gate already covers at `fail_under = 100`. - docs/doctoring/organization-commercial-readiness-loop.md — took main's. Main's paragraph is the newer one, it matches the merged workflow's own error text ("PR_REVIEW_MERGE_TOKEN or the job-bound OpenCode App token exchange is required"), and it carries a "2026-09-01 protected-main credential failure RCA" section that explicitly records this PR: the credential boundary was extracted to current main so schedule recovery would not be coupled to this larger feature, with this PR to absorb it later. This branch's paragraph adds no fact main's lacks; its explicit `NVIDIA_NIM_API_KEY` mention is covered by main's broader "model-provider keys". - CHANGELOG.md — kept both sides. One test fails after this merge and did not fail on either side alone. It is NOT caused by this resolution and is reported rather than papered over: tests/test_organization_commercial_readiness_loop_facade.py:: test_imported_facade_remains_executable_by_public_module_name unmerged head 0 failed, 2242 passed after merge 1 failed, 2909 passed (identical with -p no:randomly) Minimal deterministic reproduction, found by bisecting the suite: pytest tests/test_codeql_default_setup_rollout.py \ tests/test_organization_commercial_readiness_loop_facade.py \ -p no:randomly `test_direct_script_import_falls_back_to_sibling_module` deliberately makes `import scripts.ci.organization_commercial_readiness_loop` fail so the script takes its sibling (bare-name) fallback. `monkeypatch` restores `sys.path` and `__import__` on teardown but not `sys.modules`, so the run leaves both `organization_commercial_readiness_loop` and a bare-loader `scripts.ci.organization_commercial_readiness_loop` behind; the facade test's `runpy.run_module("scripts.ci.organization_commercial_readiness_loop")` then fails with "loader for organization_commercial_readiness_loop cannot handle scripts.ci.organization_commercial_readiness_loop". Neither test is wrong alone — they only collide once this branch and main are in the same tree. Two `monkeypatch.delitem` repairs were tried on the leaking test and neither fixed it; both were reverted rather than shipped, so `tests/test_codeql_default_setup_rollout.py` is byte-identical to main here. Coverage 100%, interrogate 100%; `ruff check --select F821` clean; workflow re-parses as valid YAML; zero conflict markers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… first
The merge of origin/main left one failing test,
`test_imported_facade_remains_executable_by_public_module_name`, reproducible
with two files and independent of test ordering:
pytest tests/test_codeql_default_setup_rollout.py \
tests/test_organization_commercial_readiness_loop_facade.py \
-p no:randomly
Root cause is in this branch's facade, not in main's test. Lines 50-54 restamp
the shared `_core` singleton's `__name__/__package__/__loader__/__spec__` on
**every** import of the facade. Measured directly:
import scripts.ci.organization_commercial_readiness_loop
-> _core.__spec__.name == "scripts.ci.organization_commercial_readiness_loop"
import organization_commercial_readiness_loop (bare sibling fallback)
-> same object, _core.__spec__.name == "organization_commercial_readiness_loop"
-> sys.modules["scripts.ci.…"] still points at that object
`runpy.run_module("scripts.ci.organization_commercial_readiness_loop")` then
asks a bare-name loader to load the dotted name and raises. Main's
`test_direct_script_import_falls_back_to_sibling_module` only supplies the
second import; it is not defective. This would break for any caller that
imports the facade under both names, with or without that test.
Two earlier `monkeypatch.delitem(sys.modules, …)` attempts were tried and
reverted: deleting the keys cannot undo the mutation, because both keys point
at the same already-restamped object.
Fix: first import wins. Stamp the public identity only while `_core` still
carries its private core spec. `sys.modules[__name__] = _core` is unchanged, so
the bare alias is still created and direct script execution still works.
Verified:
- 2-file reproduction, both orderings: 27 passed
- negative control: reverting this guard reproduces `1 failed`; restoring it
returns 27 passed
- direct execution `python organization_commercial_readiness_loop.py
--organization invalid/name` still exits 2 on the intended argument error
- full suite 2910 passed, 1 skipped, 0 failed (was 1 failed); coverage 100%;
interrogate 100%
Diagnosis and patch from the concurrent session working the same queue; the
mechanism was re-measured here before applying.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four conflicts, including one modify/delete, plus one order-dependent defect this merge surfaced. THE PERF CHANGE IS ALREADY ON MAIN. The label_section conflict in opencode_review_normalize_output.py is the same algorithm on both sides - main's copy differs only by naming the index 'idx' and carrying a Bolt comment - so main's side was taken and nothing is lost. MODIFY/DELETE, relocated rather than dropped: main deleted .github/workflows/organization-commercial-readiness-loop-quality-ci.yml in ec4521f 'ci(actions): consolidate commercial readiness quality', folding it into agent-review-runtime-quality-ci.yml. This branch had modified the deleted file to cover the two modules its split creates. Dropping that with the file would have left the coordinator ungated: loop.py is now a 54-line facade over a 892-line core plus a 274-line contract module, and the surviving gate measured only 'scripts/ci/organization_commercial_readiness_loop.py'. The same four extensions were applied to the consolidated workflow - path trigger, suite selection case, coverage --include glob, compileall list - and verified by running that job's own command: core 386/386, ddd_contract 195/195, facade 30/30, TOTAL 100%. Three contract tests pinned the literal include path and were updated to the glob with the reason in a comment (test_workflow_and_doctoring_contracts, test_commercial_readiness_suite_is_ selected_and_conditionally_executed, test_organization_loop_contract_moves_to_ agent_quality_job). Negative control: restoring the literal include makes test_workflow_and_doctoring_contracts fail by name. DEFECT FOUND AND FIXED - facade import identity: test_organization_commercial_readiness_loop_facade.py passed alone and failed in the full suite. Bisected to tests/test_codeql_default_setup_rollout.py, which prepends scripts/ci to sys.path and imports the module under its bare top-level name. Probed the resulting state directly: both sys.modules['scripts.ci.organization_commercial_readiness_loop'] and sys.modules['organization_commercial_readiness_loop'] are the SAME object and its __spec__.name is the top-level one, so runpy.run_module on the package name raises 'loader for organization_commercial_readiness_loop cannot handle scripts.ci.organization_commercial_readiness_loop'. The facade restamped _core's identity unconditionally on every import, letting the second public name overwrite the first. Guarded so only the first adopter stamps it - the same fix this repository already applied for this shape in #1545. Verified in both orders. This is the branch's facade meeting main's larger suite: the branch head passes because main's trigger test is not in its tree. Evidence: - uvx ruff check --select F821 scripts/ci tests: All checks passed - full suite, branch head 0def341 (unmerged): 2243 passed, 0 failed - full suite, this merge: 2922 passed, 0 failed - coverage: TOTAL 100%; interrogate: PASSED (minimum 100.0%) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Outcome
Reuses the existing organization commercial-readiness schedule at minute 7 of every hour and makes its product-development lane require a versioned, executable Domain-Driven Design architecture contract. No second cron or competing organization writer is introduced.
Current exact branch
76b290ca358f1b9d0e0af6df73e3429aae41b895mainincluded as second parent:44a3c740f7c46c06e7500174d4127413f3f581ebHourly execution boundary
Executable DDD contract
The repository owns its human-readable product-agent prompt, which may use any language. Eligibility no longer depends on copied English terminology appearing anywhere in raw YAML.
A valid entrypoint must define exactly one root workflow environment with:
The same non-comment product-agent command under
# cwl-ddd-prompt-binding: v1must consume both:Version one uses an exact closed capability set. Missing, extra, duplicated, misspelled, or unversioned capabilities fail closed. Comments, unrelated or unused YAML, nested or duplicate environment scopes, shell built-ins, malformed quoting, dangling continuations, and flags split across commands do not satisfy the contract.
The capabilities cover core/supporting/generic subdomain classification, Bounded Context, Context Map, Ubiquitous Language, Aggregate, Entity, Value Object, Domain Service, Repository, Domain Event, Invariant, Anti-Corruption Layer, minimal Shared Kernel, directory ownership, and
docs/product-technical-gap-baseline.mdtraceability.Directory and responsibility repair
Each hourly product increment must identify the owning product responsibility before selecting a repository, then compare the live directory tree, package/module names, API, database objects, tests, and documentation with that responsibility.
Misleading directory ownership, domain behavior hidden in generic
utilsorcommonpaths, infrastructure imports inside the domain model, cross-context database access, obsolete product names, and customer-visible implementation boundaries are architecture defects. A bounded repair moves implementation together with imports, callers, package manifests, migrations, tests, ADRs, diagrams, and compatibility adapters. A larger coherent migration must be recorded with owner, callers, target context, sequence, and acceptance evidence indocs/product-technical-gap-baseline.mdrather than silently deferred.This does not impose one universal directory template; moves are justified by domain ownership and dependency direction.
Implementation boundary
The previously verified coordinator implementation is preserved byte-for-byte in
scripts/ci/organization_commercial_readiness_core.py. The original public/script path remains a compatibility facade that loads the core and injects the new dedicated DDD contract validator. This keeps existing imports and the scheduled command stable while isolating architecture-contract parsing from fleet coordination.Coordinator credential availability
The scheduled coordinator still prefers
PR_REVIEW_MERGE_TOKEN. When absent, the protected default-branch job may exchange its job-bound GitHub OIDC identity for the existing short-lived OpenCode App installation token.Security properties:
id-token: writeonly;GITHUB_TOKEN, reviewer credential, model-provider key, orCOPILOT_GITHUB_TOKENfallback;Executable transport-level branch tests for the exchange helper are tracked separately in #1549 so pull-request code never receives a real job-bound OIDC identity.
Verification evidence
Predecessor bounded bootstrap evidence before the review remediation:
33468148830, job99732190882;compileall, andgit diff --checkpassed.Review remediation adds focused coverage for multilingual prompt freedom, exact version/capability enforcement, comments-only and unused-YAML rejection, environment scope, same-command binding, shell splitting, malformed commands, compatibility imports, and direct script mode. The permanent PR quality workflow now measures the compatibility facade, coordinator core, and DDD validator together.
All temporary source-fix workflows and scripts were removed from the final branch tree. Current-head GitHub checks and independent reviews are authoritative; predecessor evidence is supporting only.
Supersession
This current-main successor supersedes #1161 and #1233. It does not convert unavailable cross-repository credentials into a successful no-op: the loop either obtains bounded maintainer authority and performs the pass, or fails visibly.
Merge boundary
The PR remains
not-merge-ready. Merge only after the unchanged exact head has all required repository/security/coverage/review contexts successful, every actionable review thread resolved, and any policy-required independent approval. Queued, stale, predecessor-head, synthetic-merge-only, skipped-required, or failed evidence is non-passing.